1.10.0#277
Merged
digitalghost-dev merged 22 commits intomainfrom Apr 29, 2026
Merged
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
This was
linked to
issues
Apr 29, 2026
Contributor
📝 WalkthroughWalkthroughThis PR bumps the project version from v1.9.4 to v1.10.0 across build configs and documentation, updates the Go toolchain from 1.25.8 to 1.25.9, refactors the berry command to support direct CLI arguments with a new two-panel layout, improves error handling throughout multiple commands and flags, and expands test coverage with new pricing extraction, series validation, and Dagster sensor tests plus golden output fixtures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
This was
linked to
issues
Apr 29, 2026
Closed
Merging this PR will not alter performance
Performance Changes
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/berry/berryinfo.go (1)
124-133:⚠️ Potential issue | 🟠 MajorAdd timeout and HTTP status validation for image fetches.
On line 124,
http.Gethas no timeout, which can hang the CLI indefinitely. Additionally, the response status code is not validated, so HTTP errors (4xx, 5xx) would incorrectly be reported as decode errors on line 132.Suggested fix
import ( "image" "net/http" "strings" + "time" @@ - imageResp, err := http.Get(berryImage[0]) + client := &http.Client{Timeout: 10 * time.Second} + imageResp, err := client.Get(berryImage[0]) if err != nil { return "Error downloading berry image" } defer imageResp.Body.Close() + if imageResp.StatusCode != http.StatusOK { + return "Error downloading berry image" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/berry/berryinfo.go` around lines 124 - 133, Replace the plain http.Get call used to fetch berryImage[0] with an http.Client that has a timeout (e.g., time.Second * N) and use client.Get instead; after the GET, check for a non-nil imageResp and validate imageResp.StatusCode is 2xx (return an error message including the status code/body when not), then defer imageResp.Body.Close() only after confirming imageResp is non-nil, and only call imaging.Decode on imageResp.Body if the status check passed so HTTP errors aren’t misreported as decode errors.
🧹 Nitpick comments (6)
cmd/berry/berryinfo_test.go (1)
105-129:TestBerryImageWithMockServerdoesn’t exercise the mock server path.On Line 120 and Line 126, both inputs return early (
"Image information not available"), so no HTTP request is made. Consider renaming this test to an error-path test or wiring DB data to hit the mock URL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/berry/berryinfo_test.go` around lines 105 - 129, TestBerryImageWithMockServer never hits the mock HTTP server because berryImage("NonExistentBerry") and berryImage("") return the error path; either rename the test to reflect it only verifies the error path (e.g., TestBerryImage_ErrorPath) or modify the test to exercise the mock server by wiring the code that resolves image URLs to point at server.URL (for example by populating the in-memory DB/lookup used by berryImage or setting the resolver/env var it uses) and also ensure the mock handler actually writes image bytes (use png.Encode on the image before w.Write) so a real HTTP fetch succeeds; update the test to call berryImage with the berry name/key that maps to server.URL so the mock path is exercised.cmd/berry/berry_test.go (1)
269-279: Prefert.Setenvover manual set/unset for test env state.This simplifies cleanup and restores any previous value automatically.
Suggested refactor
- err := os.Setenv("GO_TESTING", "1") - if err != nil { - t.Fatalf("Failed to set GO_TESTING env var: %v", err) - } - - defer func() { - err := os.Unsetenv("GO_TESTING") - if err != nil { - t.Logf("Warning: failed to unset GO_TESTING: %v", err) - } - }() + t.Setenv("GO_TESTING", "1")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/berry/berry_test.go` around lines 269 - 279, The test currently calls os.Setenv("GO_TESTING","1") and defers os.Unsetenv, which should be replaced with t.Setenv to automatically restore prior state; update the test to remove the os.Setenv/os.Unsetenv calls and call t.Setenv("GO_TESTING", "1") (using the test's *testing.T instance `t`) in the setup so cleanup is handled for you.structs/structs.go (1)
145-148: Add a focused unmarshalling test forCriesfields.Since this new field powers
--cry, add aPokemonApiCalltest assertion forCries.Latest/Cries.Legacyto prevent silent schema regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@structs/structs.go` around lines 145 - 148, Add a focused unit test that unmarshals a sample JSON containing the Cries object and asserts Cries.Latest and Cries.Legacy values to prevent schema regressions; specifically create or extend the PokemonApiCall test to unmarshal into the Cries struct (type Cries) and include assertions that compare the expected strings to the unmarshalled Cries.Latest and Cries.Legacy fields so the test fails if those keys are missing or renamed.flags/cry.go (1)
34-38: HTTP GET lacks timeout, risking indefinite hang.
http.Getuses the default client with no timeout. If the cry URL server is slow or unresponsive, this could hang indefinitely.♻️ Add a timeout
- resp, err := http.Get(cryURL) + client := &http.Client{Timeout: 15 * time.Second} + resp, err := client.Get(cryURL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flags/cry.go` around lines 34 - 38, The HTTP GET to cryURL uses http.Get (the default client) with no timeout; replace it with a request that has a timeout to avoid hanging. Create an http.Client with a sensible Timeout (e.g., &http.Client{Timeout: 10 * time.Second}) and use client.Get(cryURL) or build the request with context.WithTimeout and http.NewRequestWithContext, handle the error as before, and defer resp.Body.Close() after ensuring resp is non-nil; update imports (time or context) and adjust the call sites around cryURL accordingly.card_data/pyproject.toml (2)
3-3: Non-standard Python version format.The
vprefix in version"v1.10.0"doesn't conform to PEP 440. While pip may tolerate this, some tools and registries may have issues. Consider using"1.10.0"for consistency with Python packaging standards.♻️ Suggested fix
-version = "v1.10.0" +version = "1.10.0"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@card_data/pyproject.toml` at line 3, The version string in pyproject.toml uses a non-PEP 440 format ("v1.10.0"); update the value to a PEP 440-compliant string (remove the "v") so it reads "1.10.0" to avoid tooling/registry problems—edit the version = "v1.10.0" entry in pyproject.toml to version = "1.10.0".
22-22:pytest-covbelongs in dev dependencies, not main dependencies.
pytest-covis a testing tool and should be in the[dependency-groups] devsection alongsidepytest(line 35), not in the maindependencieslist. Adding it to runtime dependencies unnecessarily bloats production installs.♻️ Move to dev group
"pydantic==2.11.7", - "pytest-cov==7.1.0", "requests==2.32.4",Then add to the dev group:
[dependency-groups] dev = [ ... "pytest==9.0.2", "pytest-cov==7.1.0", ... ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@card_data/pyproject.toml` at line 22, Remove "pytest-cov==7.1.0" from the main dependencies list in pyproject.toml and add it to the dev dependency group alongside pytest (i.e., the [dependency-groups] dev section), ensuring the entry name matches the existing style ("pytest-cov==7.1.0") so it is only installed for development/testing and not at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@card_data/pipelines/poke_cli_dbt/dbt_project.yml`:
- Line 2: The dbt_project.yml uses a non-compliant version string "v1.10.0";
update the version key value to plain SemVer by removing the leading "v" so it
reads 1.10.0 (i.e., change the version entry from 'v1.10.0' to '1.10.0') to
satisfy dbt's version format requirements.
In `@card_data/pipelines/tests/extract_pricing_test.py`:
- Line 22: The test incorrectly accesses a non-existent attribute path on the
Dagster asset by using build_dataframe.op.compute_fn.decorated_fn; remove that
invalid access and either call the asset directly (invoke build_dataframe() to
get its return value) or, if the asset needs a Dagster context, use Dagster
testing utilities such as build_asset_context() to call build_dataframe(context)
or dg.materialize_to_memory([build_dataframe]) for integration-style testing;
update the variable _build_dataframe_fn to use one of these valid invocation
approaches referencing the build_dataframe symbol.
In `@card_data/pipelines/tests/sensors_test.py`:
- Around line 12-13: Replace direct accesses to the private attribute
`_run_status_sensor_fn` for `discord_success_sensor` and
`discord_failure_sensor` with Dagster's public test helper: call
`build_run_status_sensor_context()` to create a run-status sensor context (use
ephemeral_instance=True or other options you need) and then invoke the sensor
with that context (e.g., call
`discord_success_sensor(build_run_status_sensor_context(...))` and
`discord_failure_sensor(build_run_status_sensor_context(...))`), asserting on
the returned sensor evaluation; this removes fragile private API usage and uses
Dagster's supported testing API.
In `@cmd/berry/berry_test.go`:
- Around line 319-323: The test currently discards the error return from
BerryCommand(); change the call to capture both output and err (e.g., output,
err := BerryCommand()), then assert the command succeeded before comparing
outputs by using t.Fatalf or assert.NoErrorf(t, err, "BerryCommand failed: %v",
err) (or assert.NoError) so failures in BerryCommand() are surfaced, and only
then strip ANSI and compare cleanOutput to tt.expectedOutput; reference
BerryCommand() and the test's output/cleanOutput variables when making the
change.
In `@cmd/berry/berry.go`:
- Around line 39-55: The direct-query branch currently checks len(os.Args) > 2
which allows extra positional args (e.g. "berry cheri extra"); change the logic
to require exactly one berry name by validating len(os.Args) == 3 (or reject
when len(os.Args) > 3) and/or tighten utils.ValidateArgs (set MaxArgs to 3
and/or RequireName true) so extra args are rejected; update the branch that
reads berryName (styling.CapitalizeResourceName(os.Args[2])) to only run when a
single name is provided, and return a formatted error via utils.FormatError when
extra positional arguments are present; keep calls to berryExists and
berryContainers unchanged.
In `@cmd/berry/berryinfo.go`:
- Around line 14-21: The function berryExists should not swallow database
errors; change berryExists to return (bool, error) instead of bool, have it call
connections.QueryBerryData(...) and return (len(results) > 0, err) so real DB
errors propagate, and update any callers to handle the error (e.g. check error
before assuming "not found"); reference the berryExists function and the
connections.QueryBerryData call when making the change.
In `@cmd/card/cardinfo.go`:
- Around line 84-87: The error handling around url.Parse(imageURL) should
distinguish parse failures from invalid schemes: if url.Parse returns an error
(err != nil) propagate or wrap that parse error (including err) rather than
returning the generic API message, and if parsing succeeds but parsedURL.Scheme
is not "http" or "https" return a more specific error (e.g., "invalid URL
scheme" and include parsedURL.Scheme or the original imageURL) so callers can
tell it's client-side URL validation; update the logic around parsedURL, err and
the returned errors accordingly.
In `@cmd/types/damage_table_test.go`:
- Around line 22-24: The test redirects os.Stdout before calling DamageTable but
uses t.Fatalf which can terminate the test before stdout is restored; to fix,
capture the original stdout (e.g., old := os.Stdout) and register a guaranteed
restore using t.Cleanup(func(){ os.Stdout = old }) (or defer immediately after
redirect) before invoking DamageTable so stdout is restored even on fatal paths;
update the test around the DamageTable("fire", "type") call to use this cleanup
and keep the existing failure assertion.
In `@flags/cry.go`:
- Line 29: The error string passed to cmdutils.FormatError in the return
statement currently concatenates "No cry available for"+pokemonName without a
space; update the return in the function that constructs this error (the line
using fmt.Errorf and cmdutils.FormatError) to include a space before the
pokemonName (e.g., "No cry available for " + pokemonName or use fmt.Sprintf) so
the message reads "No cry available for <pokemonName>" and keep using
FormatError and fmt.Errorf as before.
- Around line 50-62: The pcm allocation can panic if dec.Length() returns -1;
update the code around the pcm slice creation (symbols: dec.Length(),
dec.Channels(), pcm, dec.Read) to detect when dec.Length() < 0 and handle
unknown length by using a resizable approach (e.g., start with a safe initial
capacity and append reads or incrementally grow the buffer) instead of
allocating a fixed-size slice with a negative length; ensure the read loop still
uses dec.Read(pcm[read:]) style when length is known, and switches to
appending/expanding pcm when length is unknown so reads continue until io.EOF
without causing a negative-size panic.
In `@README.md`:
- Around line 102-103: Add a clear note to the Docker usage section (near the
docker run example for digitalghostdev/poke-cli:v1.10.0) stating that the --cry
flag is unavailable in the container because the image is built with -tags nocry
in the Dockerfile; update the docker command block and related Docker section
text (the paragraphs around the docker run example and the other referenced
Docker lines) to explicitly mention “--cry not supported in Docker images (built
with -tags nocry)” so users won’t expect in-container cry playback.
---
Outside diff comments:
In `@cmd/berry/berryinfo.go`:
- Around line 124-133: Replace the plain http.Get call used to fetch
berryImage[0] with an http.Client that has a timeout (e.g., time.Second * N) and
use client.Get instead; after the GET, check for a non-nil imageResp and
validate imageResp.StatusCode is 2xx (return an error message including the
status code/body when not), then defer imageResp.Body.Close() only after
confirming imageResp is non-nil, and only call imaging.Decode on imageResp.Body
if the status check passed so HTTP errors aren’t misreported as decode errors.
---
Nitpick comments:
In `@card_data/pyproject.toml`:
- Line 3: The version string in pyproject.toml uses a non-PEP 440 format
("v1.10.0"); update the value to a PEP 440-compliant string (remove the "v") so
it reads "1.10.0" to avoid tooling/registry problems—edit the version =
"v1.10.0" entry in pyproject.toml to version = "1.10.0".
- Line 22: Remove "pytest-cov==7.1.0" from the main dependencies list in
pyproject.toml and add it to the dev dependency group alongside pytest (i.e.,
the [dependency-groups] dev section), ensuring the entry name matches the
existing style ("pytest-cov==7.1.0") so it is only installed for
development/testing and not at runtime.
In `@cmd/berry/berry_test.go`:
- Around line 269-279: The test currently calls os.Setenv("GO_TESTING","1") and
defers os.Unsetenv, which should be replaced with t.Setenv to automatically
restore prior state; update the test to remove the os.Setenv/os.Unsetenv calls
and call t.Setenv("GO_TESTING", "1") (using the test's *testing.T instance `t`)
in the setup so cleanup is handled for you.
In `@cmd/berry/berryinfo_test.go`:
- Around line 105-129: TestBerryImageWithMockServer never hits the mock HTTP
server because berryImage("NonExistentBerry") and berryImage("") return the
error path; either rename the test to reflect it only verifies the error path
(e.g., TestBerryImage_ErrorPath) or modify the test to exercise the mock server
by wiring the code that resolves image URLs to point at server.URL (for example
by populating the in-memory DB/lookup used by berryImage or setting the
resolver/env var it uses) and also ensure the mock handler actually writes image
bytes (use png.Encode on the image before w.Write) so a real HTTP fetch
succeeds; update the test to call berryImage with the berry name/key that maps
to server.URL so the mock path is exercised.
In `@flags/cry.go`:
- Around line 34-38: The HTTP GET to cryURL uses http.Get (the default client)
with no timeout; replace it with a request that has a timeout to avoid hanging.
Create an http.Client with a sensible Timeout (e.g., &http.Client{Timeout: 10 *
time.Second}) and use client.Get(cryURL) or build the request with
context.WithTimeout and http.NewRequestWithContext, handle the error as before,
and defer resp.Body.Close() after ensuring resp is non-nil; update imports (time
or context) and adjust the call sites around cryURL accordingly.
In `@structs/structs.go`:
- Around line 145-148: Add a focused unit test that unmarshals a sample JSON
containing the Cries object and asserts Cries.Latest and Cries.Legacy values to
prevent schema regressions; specifically create or extend the PokemonApiCall
test to unmarshal into the Cries struct (type Cries) and include assertions that
compare the expected strings to the unmarshalled Cries.Latest and Cries.Legacy
fields so the test fails if those keys are missing or renamed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba71fdd0-d43e-46db-8047-a4e9745882be
⛔ Files ignored due to path filters (2)
card_data/uv.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
.github/workflows/ci.yml.gitignore.goreleaser.ymlDockerfileREADME.mdcard_data/pipelines/poke_cli_dbt/dbt_project.ymlcard_data/pipelines/tests/extract_pricing_test.pycard_data/pipelines/tests/extract_series_test.pycard_data/pipelines/tests/sensors_test.pycard_data/pyproject.tomlcmd/berry/berry.gocmd/berry/berry_test.gocmd/berry/berryinfo.gocmd/berry/berryinfo_test.gocmd/card/cardinfo.gocmd/pokemon/pokemon.gocmd/pokemon/render_test.gocmd/types/damage_table.gocmd/types/damage_table_test.gocmd/types/types.goflags/abilityflagset.goflags/abilityflagset_test.goflags/cry.goflags/cry_nocry.goflags/cry_test.goflags/pokemonflagset.gogo.modnfpm.yamlstructs/structs.gotestdata/berry_aguav.goldentestdata/berry_cheri.goldentestdata/berry_chople.goldentestdata/berry_oran.goldentestdata/berry_sitrus.goldentestdata/main_latest_flag.goldenweb/pyproject.toml
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/berry/berry.go (1)
39-45:⚠️ Potential issue | 🟠 MajorReject trailing positional args in direct-query mode.
Line 39 still allows four argv entries, and Line 44 still branches on
len(os.Args) > 2, soberry cheri extrais accepted and silently ignoresextra. Tighten this path so exactly one berry name is allowed.Suggested fix
- if err := utils.ValidateArgs(os.Args, utils.Validator{MaxArgs: 4, CmdName: "berry", RequireName: false, HasFlags: false}); err != nil { + if err := utils.ValidateArgs(os.Args, utils.Validator{MaxArgs: 3, CmdName: "berry", RequireName: false, HasFlags: false}); err != nil { output.WriteString(err.Error()) return output.String(), err } - if len(os.Args) > 2 { + if len(os.Args) == 3 { berryName := styling.CapitalizeResourceName(os.Args[2])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/berry/berry.go` around lines 39 - 45, The direct-query path currently permits extra positional args; update the validator and branch to require exactly one berry name: change the utils.ValidateArgs call to use Validator{MaxArgs: 3, RequireName: true, CmdName: "berry", HasFlags: false} so trailing args are rejected, and replace the branch condition from if len(os.Args) > 2 to if len(os.Args) == 3 before calling styling.CapitalizeResourceName(os.Args[2]) so only a single berry name is accepted.
🧹 Nitpick comments (1)
card_data/pipelines/tests/extract_pricing_test.py (1)
1-4: Avoid mutatingsys.pathat module import time.Line 4 prepends a dynamic path globally, which can leak across the test session and shadow installed modules. Prefer configuring test import roots in
pyproject.toml/pytest.ini(e.g., pytestpythonpath) or using an installable package layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@card_data/pipelines/tests/extract_pricing_test.py` around lines 1 - 4, Remove the top-level mutation of sys.path (the statement sys.path.insert(0, str(Path(__file__).parent.parent.parent))) and instead either configure the test import root via pytest (add pythonpath/settings in pytest.ini or pyproject.toml) or, if a temporary path change is absolutely required for a specific test, perform it locally inside that test using pytest's monkeypatch (monkeypatch.syspath_prepend or monkeypatch.setitem on sys.path) so the change is scoped to the test run rather than at module import time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/berry/berry.go`:
- Around line 39-45: The direct-query path currently permits extra positional
args; update the validator and branch to require exactly one berry name: change
the utils.ValidateArgs call to use Validator{MaxArgs: 3, RequireName: true,
CmdName: "berry", HasFlags: false} so trailing args are rejected, and replace
the branch condition from if len(os.Args) > 2 to if len(os.Args) == 3 before
calling styling.CapitalizeResourceName(os.Args[2]) so only a single berry name
is accepted.
---
Nitpick comments:
In `@card_data/pipelines/tests/extract_pricing_test.py`:
- Around line 1-4: Remove the top-level mutation of sys.path (the statement
sys.path.insert(0, str(Path(__file__).parent.parent.parent))) and instead either
configure the test import root via pytest (add pythonpath/settings in pytest.ini
or pyproject.toml) or, if a temporary path change is absolutely required for a
specific test, perform it locally inside that test using pytest's monkeypatch
(monkeypatch.syspath_prepend or monkeypatch.setitem on sys.path) so the change
is scoped to the test run rather than at module import time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8e43b3e-bec8-44ec-bdbd-e344dcffa81e
⛔ Files ignored due to path filters (1)
card_data/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/ci.ymlDockerfilecard_data/pipelines/tests/extract_pricing_test.pycmd/berry/berry.gocmd/berry/berry_test.gocmd/berry/berryinfo.goflags/pokemonflagset.gogo.mod
✅ Files skipped from review due to trivial changes (2)
- go.mod
- Dockerfile
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/berry/berry_test.go
- .github/workflows/ci.yml
- flags/pokemonflagset.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests